fix: convert incorrect breadcrumb links to opaque spans #7626
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
dario-piotrowicz
force-pushed
the
dario/fix-breadcrumb-links
branch
from
19d7a4d to
08222ad
Compare
github-actions
bot
removed
the
github_actions:pull-request
|
Can you add a linkless story? |
|
Lighthouse Results
|
Unit Test Coverage Report
|
Thanks for the suggestion 🙂 Sure, done 🙂👍 |
|
mh... actually I might need to amend something, the last item should be styled as active even if it doesn't have an
The |
addressed 👍 614899a (the solution is a bit ugly but I am not sure if much better can be done here 🤔) |
It's because of the site navigation structure, for example see:
there is actually no so if the breadcrumb for |
|
Right, but I do have the feeling that something else is at play here 🤔 like all these breadcrumbs should link to actual pages. They should correlate to something within the tree of the path structure. So something could be borken on the breadcrumb generation. |
mh... I feel like this is a UI/design issue more than a code related one, so if we again take https://nodejs.org/en/learn/getting-started/differences-between-nodejs-and-the-browser as an example and you look at the breadcrumbs at the bottom, what do you think they should look like? should If I feel like we first need to answer these design questions before we can agree/understand if there's something actually wrong with the code |
We should make the first article of getting-started also be available under /getting-started, that could fix this issue (we can make internal redirects on redirects.json) wdyt? |
We can open a follow-up issue for this if you feel like? |
I did try something like that and it worked pretty ok however this was a bit awkward in the first article since the link there would point to the exact same page, so let's take https://nodejs.org/en/learn/getting-started/introduction-to-nodejs there you'd have: where the last two breadcrumb links would point to the exact same page, which is not the end of the world but if does feel pretty awkward to me 🤔 |
Sorry but I feel like the solution here and what you're suggesting are not really compatible (we either do make the breadcrumb a valid working link or make it not a link at all, I don't think that we can apply both). If you prefer your solution I am totally happy to just close this PR and open a new one with that solution instead 🙂 (however please do keep in mind my comment above) |
Description
It's pretty minor but I figured I could just open a PR to fix this small UI bug I noticed 🙂
The fact is that in pages with breadcrumbs at the bottom of the screen most of the time the first breadcrumbs link is not really a valid link and clicking it results in a hard reload of the app:
See:
My changes here make breadcrumb items with falsy hrefs simple text elements instead of a links, for good measure they are also dimmed to make it more evident that they are not clickable
Validation
I manually validated this check with
npm run devand alsonpm run build && npm run startCheck List
npm run formatto ensure the code follows the style guide.npm run testto check if all tests are passing.npx turbo buildto check if the website builds without errors.